[Enhancement] (FE) Convert intra FE-to-FE calls to HTTPS when enabled#60921
Open
nsivarajan wants to merge 7 commits intoapache:masterfrom
Open
[Enhancement] (FE) Convert intra FE-to-FE calls to HTTPS when enabled#60921nsivarajan wants to merge 7 commits intoapache:masterfrom
nsivarajan wants to merge 7 commits intoapache:masterfrom
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
nsivarajan
requested review from
CalvinKirs,
dataroaring and
morningman
as code owners
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 28728 ms |
TPC-DS: Total hot run time: 183423 ms |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29084 ms |
TPC-DS: Total hot run time: 184255 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Collaborator
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR converts intra FE-to-FE HTTP communication to HTTPS when enable_https=true. The core approach is sound — centralizing URL construction in HttpURLUtil.buildInternalFeUrl() and adding SSL-aware HTTP clients in InternalHttpsUtils. However, I found several issues that should be addressed:
Critical Issues
-
JVM-wide global state mutation (
installTrustManagerForUrlConnection) —setDefaultSSLSocketFactory()andsetDefaultHostnameVerifier()modify JVM-wide defaults that affect ALLHttpsURLConnectioninstances in the process, not just FE-to-FE connections. This is called on everygetConnectionWithNodeIdent()invocation, which is wasteful and thread-unsafe. Additionally, the hostname verifier(hostname, session) -> truecompletely disables hostname verification globally, which could affect other HTTPS connections in the JVM (e.g., external catalog connections). -
Resource leak in
HttpUtils.executeRequest()—CloseableHttpClientis created but never closed. TheResponseHandlervariant ofexecute()closes the HTTP response, but NOT the client itself. This leaks connection pool resources. Other callers in the codebase (e.g.,MysqlLoadManager,HFUtils) properly usetry-with-resources. -
Incomplete conversion — Several FE-to-FE communication paths still hardcode
http://andConfig.http_port, which will break whenenable_https=trueandhttp_port=0. Key missed locations:HttpUtils.concatUrl()(line 75) — hardcodeshttp://HttpUtils.getFeList()(line 65) — hardcodesConfig.http_portSessionController.getOtherSessionInfo()— hardcodeshttp://NodeAction.concatFeSetConfigUrl()— hardcodeshttp://NodeAction.getFeList()andhandleConfigurationInfo()— hardcodeshttp://andConfig.http_port
Minor Issues
-
Removed useful comments — Two comments about using
Env.getServingEnv()instead ofgetCurrentEnv()were deleted. These documented a non-obvious requirement and should be preserved. -
Removed LOG.info in MetaHelper.doGet — The debug logging for URL, timeout, and headers was removed. This is useful for troubleshooting connectivity issues.
Checkpoints
- Correctness: Issues found (global state mutation, incomplete conversion, resource leak)
- Security: The approach of certificate validation + disabled hostname verification for internal communication is reasonable and documented, but the JVM-wide global mutation is a concern
- Performance:
installTrustManagerForUrlConnection()re-reads the truststore from disk and re-initializes the SSLContext on every call — should be cached/initialized once - Tests: Unit tests for
buildInternalFeUrl()are good; no tests for the SSL client creation or the HTTPS integration path
fe/fe-core/src/main/java/org/apache/doris/common/util/InternalHttpsUtils.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/manager/HttpUtils.java
Outdated
Show resolved
Hide resolved
fe/fe-core/src/main/java/org/apache/doris/common/util/InternalHttpsUtils.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 29027 ms |
TPC-DS: Total hot run time: 184297 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
@Hastyshell @dataroaring @morningman @CalvinKirs - Could you please help with review and get this merged ! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Currently, Doris provides an enable_https switch to enforce HTTPS connections. In hardened deployments, HTTP is completely disabled by setting http_port = 0. However, intra FE-to-FE communication still relies on HTTP, causing failures in edit log and checkpoint synchronisation when HTTPS is enabled.
This PR enhances HTTPS support by automatically converting intra FE-to-FE communication to HTTPS when enable_https=true, without introducing any new configuration, leveraging mysql_ssl_default_ca_certificate created for https connection. When enable_https=false, behavior remains unchanged. This ensures secure and seamless FE-to-FE communication without breaking existing workflows.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)